fix(security): 2 improvements across 2 files#228
Open
tomaioo wants to merge 2 commits intoOpenCoworkAI:mainfrom
Open
fix(security): 2 improvements across 2 files#228tomaioo wants to merge 2 commits intoOpenCoworkAI:mainfrom
tomaioo wants to merge 2 commits intoOpenCoworkAI:mainfrom
Conversation
- Security: TOCTOU race in defensive file reader can bypass type/size checks - Security: Unbounded IPC payload size for export can enable renderer-to-main DoS Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
- Security: TOCTOU race in defensive file reader can bypass type/size checks - Security: Unbounded IPC payload size for export can enable renderer-to-main DoS Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
github-actions
Bot
added
the
area:desktop
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Findings
- [Blocker]
open()can hang on FIFOs before the new regular-file check runs —safeReadImportFile()now callsopen(path, 'r')beforefileHandle.stat(), so a path swapped to a named pipe will block the Electron main process instead of returningnull. I reproduced this on the Linux runner withmkfifoplusfs.promises.open(), which timed out after 2s. Evidence:apps/desktop/src/main/imports/safe-read.ts:32.
Suggested fix:This keeps the single-descriptor flow that fixes the TOCTOU issue, but avoids blocking on FIFO/socket-style paths beforeimport { constants } from 'node:fs'; import { open } from 'node:fs/promises'; const flags = constants.O_RDONLY | (constants.O_NONBLOCK ?? 0); fileHandle = await open(path, flags);
fileHandle.stat()can reject them.
Summary
- Review mode: initial
- 1 blocker found in the new
safeReadImportFile()flow. The TOCTOU fix is directionally right, but the currentopen(path, 'r')ordering reintroduces a main-process DoS path for named pipes atapps/desktop/src/main/imports/safe-read.ts:32.
Testing
- Not run (automation). Suggested regression tests: add a POSIX-only
safe-readcase proving a FIFO path returnsnullwithout blocking; add aparseRequest()case for oversizedhtmlContent.
open-codesign Bot
| let fileHandle: Awaited<ReturnType<typeof open>>; | ||
| try { | ||
| stats = await stat(path); | ||
| fileHandle = await open(path, 'r'); |
Contributor
There was a problem hiding this comment.
open(path, 'r') runs before the regular-file check, so a contributor can replace this path with a FIFO and block the main process before fileHandle.stat() executes. On this Linux runner, mkfifo + fs.promises.open() hung until timeout killed it.
Suggested fix:
import { constants } from 'node:fs';
import { open } from 'node:fs/promises';
const flags = constants.O_RDONLY | (constants.O_NONBLOCK ?? 0);
fileHandle = await open(path, flags);That preserves the single-handle TOCTOU fix while preventing FIFO/socket-style paths from hanging the process.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fix(security): 2 improvements across 2 files
Problem
Severity:
Medium| File:apps/desktop/src/main/imports/safe-read.ts:L34safeReadImportFileperformsstat(path)to validate file type and size, then separately callsreadFile(path). An attacker with local write access to the same filesystem location can race-replace the file between these calls (e.g., swap a small regular file for a FIFO/device/very large file), bypassing the earlier validation and potentially causing hangs or memory pressure in the main process.Solution
Open the file once and validate/read via the same file descriptor to avoid path re-resolution races. For example:
const fh = await open(path, O_RDONLY | O_NOFOLLOW)(where supported), thenconst st = await fh.stat(), enforcest.isFile()and size cap, thenawait fh.readFile('utf8'), finally close handle infinally.Changes
apps/desktop/src/main/imports/safe-read.ts(modified)apps/desktop/src/main/exporter-ipc.ts(modified)